Cleanup alternate storage location keys of user#16
Conversation
sharidas
force-pushed
the
listen-event-delete-userkeys-alt-storage
branch
from
c606dae to
837bc98
Compare
sharidas
changed the title
| $this->keyManager->deletePublicKey($params['uid']); | ||
| } | ||
|
|
||
| \OC::$server->getEncryptionKeyStorage()->deleteAltUserStorageKeys($params['uid']); |
There was a problem hiding this comment.
Could the key storage be injected?
There was a problem hiding this comment.
Should this piece be executed only if the encryption app is enable? (just asking)
|
Specify what you've tested in the comments in case there are some scenarios that you didn't consider. Some questions I have:
|
|
So the behaviour without this change:
Do we really need to check if the uid exists, if its null/empty? Because all we are trying to achieve here is a simple remove operation if : the storage location is not default one. |
According to what I see in https://github.com/owncloud/core/blob/17691e444641a84103387ef118beee9b0df4f112/lib/private/Encryption/Keys/Storage.php#L197 if somehow the user uid is empty, it could imply that all the user keys are deleted. This is really bad. |
sharidas
force-pushed
the
listen-event-delete-userkeys-alt-storage
branch
from
837bc98 to
8688c95
Compare
|
I have verified both using UI and command to delete user, uid wasn't found null/empty or so. You are right, if the uid becomes empty then it would delete the entire folder. So I have added a check before calling the method to make sure its not null or empty. |
| $this->keyManager->deletePublicKey($params['uid']); | ||
| } | ||
|
|
||
| if (!is_null($params['uid']) && ($params['uid'] !== '')) { |
There was a problem hiding this comment.
I think it's better to move this inside the App::isEnabled('encryption') check, just for consistency. Maybe we can remove the App::isEnabled('encryption') check and remove the keys in any case. It's weird that some keys are removed if the encryption is disabled but others not.
If this is the right behaviour, add a comment explaining.
There was a problem hiding this comment.
Moved the piece of code inside App::isEnabled('encryption') check. Since its a hook, imho it would not be a good idea to remove the check App::isEnabled('encryption').
This is to make sure that there are no user specific folders if the keys are stored in alternate location apart from the default location, when a user is deleted. Hence the keys inside user folder in the alternate location would be deleted. Signed-off-by: Sujith H <sharidasan@owncloud.com>
sharidas
force-pushed
the
listen-event-delete-userkeys-alt-storage
branch
from
8688c95 to
d3a9f95
Compare
3 participants
This is to make sure that there are no user
specific folders if the keys are stored in
alternate location apart from the default
location, when a user is deleted. Hence the
keys inside user folder in the alternate
location would be deleted.
Signed-off-by: Sujith H sharidasan@owncloud.com